-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add local build API for direct filesystem builds on MacOS and Windows #27059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9e81b73
to
46d2fc6
Compare
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
cf65a4c
to
25cc198
Compare
f4c1c04
to
f19d0f3
Compare
code LGTM ... if you get asked to fix things by others, adding a little commentary inside some of those functions might help the next person. again only if you have to repush and have time. |
internal/localapi/utils.go
Outdated
result[path] = mapping.RemotePath | ||
} | ||
|
||
logrus.Debugf("Translation map: %#v\n", result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this ? This message has no context. Translation Map for what.
func validateLocalPath(path string) error { | ||
if !filepath.IsAbs(path) { | ||
return fmt.Errorf("path %q is not absolute", path) | ||
} | ||
|
||
if err := fileutils.Exists(path); err != nil { | ||
return err | ||
} | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have this function elsewhere as well, could we do a quick search before adding this again ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find a similar function, but I moved it to the local API to be provided to others in the future.
return nil, utils.GetInternalServerError(fmt.Errorf("additionalbuildcontexts must be in name=value format: %q", url)) | ||
} | ||
|
||
logrus.Debugf("name: %q, context: %q", name, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message needs more context
pkg/bindings/images/build.go
Outdated
if isStdIn { | ||
stdinFile, err := tempManager.CreateTempFileFromReader(contextDir, "podman-build-stdin-*", os.Stdin) | ||
if err != nil { | ||
return nil, fmt.Errorf("processing stdin: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("processing stdin: %w", err) | |
return nil, fmt.Errorf("processing containerfile from stdin: %w", err) |
f19d0f3
to
aa36c82
Compare
} | ||
|
||
for _, c := range containerFiles { | ||
// Don not add path to containerfile if it is a URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Don not add path to containerfile if it is a URL | |
// Do not add path to containerfile if it is a URL |
Nit
pkg/bindings/images/build.go
Outdated
remoteContainerfile, ok = translationLocalAPIMap[containerfile] | ||
if !ok { | ||
if !isInContextDir { | ||
return nil, fmt.Errorf("containerfile %q not found in translation map", containerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("containerfile %q not found in translation map", containerfile) | |
return nil, fmt.Errorf("Containerfile %q not found in translation map", containerfile) |
I'll just add this one and it's a nit, but a big nit. Customer facing verbaiage should list this with a capital C
. I'll just comment here, but there are other intances.
LGTM |
aa36c82
to
5a5d58c
Compare
… (only WSL) Adds /libpod/local/build endpoint, client bindings, and path translation utilities to enable container builds from mounted directories to podman machine without tar uploads. This optimization significantly speeds up build operations when working with remote Podman machines by eliminating redundant file transfers for already-accessible files. Fixes: https://issues.redhat.com/browse/RUN-3249 Signed-off-by: Jan Rodák <[email protected]>
Replace single provider.Get() with provider.GetAll() loop to search across all available machine providers when finding machine by port. Signed-off-by: Jan Rodák <[email protected]>
5a5d58c
to
092aaeb
Compare
I fixed the nits and rebased on the latest |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flouthoc, Honny1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Happy Green test buttons! |
// description: | | ||
// Absolute path to the build context directory on the server filesystem. | ||
// This directory must contain all files needed for the build. | ||
// - in: query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sucks a bit in terms of duplication, people are already forgetting to update swagger docs now having two do two places for each build param is gonna make this worse and cause more drift.
But since I am not really aware of any swagger tricks in that regard I guess we just have to live with that for now.
contextRemotePath, found := translationLocalAPIMap[options.ContextDirectory] | ||
if !found { | ||
return nil, fmt.Errorf("cannot access context directory %q for local build", options.ContextDirectory) | ||
} | ||
requestParts.Params.Set("localcontextdir", contextRemotePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not make more sense to remove translationLocalAPIMap?
I don't see what purpose a normal bindings user who get out of it, one could just set options.ContextDirectory
to the right value from the beginning same for the AdditionalBuildContexts.
We could just do all the translations in the BuildOptions already in pkg/domain/infra/tunnel instead and not bother the actual bindings with such details.
// build contexts. Missing mappings will result in build errors. | ||
// | ||
// Returns a BuildReport containing the final image ID and save format. | ||
func BuildLocal(ctx context.Context, containerFiles []string, options types.BuildOptions, translationLocalAPIMap map[string]string) (*types.BuildReport, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming is misleading for external consumer of the bindings, a naive reader could assume this means build locally on the current system not on the server via API.
The comment is clear the code reviewer just seeing the function names would likely assume something else.
Something like BuildFromServerContext
might be more clear that the paths must exist on the server not the client.
Adds
/libpod/local/build
endpoint, client bindings, and path translation utilities to enable container builds from mounted directories to podman machine without tar uploads.This optimization significantly speeds up build operations when working with remote Podman machines by eliminating redundant file transfers for already-accessible files.
This feature is non-functional on Windows when using the Hyper-V provider. The issue stems from the 9p filesystem protocol, which fails to correctly translate file attributes between the host and the virtual machine. This incompatibility causes certain container build operations to fail. As a result, this functionality has been disabled for Windows environments that utilize Hyper-V.
Fixes: https://issues.redhat.com/browse/RUN-3249
Fixes Build part of #26321
Benchmark
podman build -t test ./test-context
Benchmark Results:
Mac OS
Providers
applehv
libkrun
Windows
Providers
WSL
Hyper-V
Does this PR introduce a user-facing change?